Expo support upgrade by itoys · Pull Request #478 · microsoft/vscode-react-native

itoys · 2017-07-18T12:52:28Z

…tive into expo-support-upgrade

msftclas · 2017-07-18T12:52:32Z

@itoys,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

vladimir-kotikov · 2017-07-25T09:50:52Z

-        const reactCommand = HostPlatform.getNpmCliCommand(CommandExecutor.ReactNativeCommand);
-        return this.spawnChildProcess(reactCommand, this.combineArguments(command, args), options);
+        const reactCommand = CommandExecutor.ReactNativeCLI;
+        return this.spawnChildProcess("node", [reactCommand, command, ...args], options);


@itoys is it really required to spawn cli.js directly rather than via react-native-cli?

That's exactly like exp run builder

Packager, you meant. Anyway, this call is effectively has the same result as this.spawnChildProcess(reactCommand, this.combineArguments(command, args), options) so I wonder if the change was really required.

vladimir-kotikov · 2017-07-25T09:51:05Z

        return Q.reject<void>(ErrorHelper.getNestedError(reason, InternalErrorCode.CommandFailed, command));
    }
-
-    private combineArguments(firstArgument: string, otherArguments: string[] = []) {


vladimir-kotikov · 2017-07-25T09:54:24Z

                    return this.monkeyPatchOpnForRNPackager()
                        .then(() => {
-                            let args = ["--port", port.toString()];
+                            let args: any = ["--port", port.toString()];


any -> string[]

But it can contain arrays
See

It shouldn't contain anything other than strings - semantically args is an array of parameters passed to the packages and commandline parameters are always strings.

The line that you pointing me to (args.push("--assetExts", ["ttf"])) is also very confusing and probably would look better if rewritten to args.push("--assetExts", ["ttf"].toString())

Another argument is that I perfectly remember that you've had a lot of troubles you could avoid if array value was converted to string explicitly

child_process.spawn is flatted nested arrays, we can get array from app.json or exp.json
Otherwise we should check typeof value and join it (Array.prototype.join)

vladimir-kotikov · 2017-07-25T09:57:43Z

+
+declare class ExpConfig {
+    public name: string;
+    public slug: string;


Are name and slug required? IIRC CRNA app doesn't have them defined in package.json

See Expo docs

Ok, makes sense

vladimir-kotikov · 2017-07-25T10:21:02Z

    let commandExecutor = new CommandExecutor();
    xdlPackage = commandExecutor.spawnWithProgress(HostPlatform.getNpmCliCommand("npm"),
-        ["install", `xdl@${XDL_VERSION}`, "--verbose"],
+        ["install", EXPO_DEPS.join(", "), "--verbose"],


Please don't use , to join CLI arguments. The common practice is to delimit using just space

vladimir-kotikov · 2017-07-25T10:31:30Z

-                         })
+                            args.push("--root", path.resolve(this.projectPath + "/.vscode/"));
+
+                            let helper = new ExponentHelper(this.projectPath);


For these two lines please see my comment to rn-extension.ts and #359

vladimir-kotikov · 2017-07-25T10:46:59Z

     */
    private pathToFileInWorkspace(filename: string): string {
-        return path.join(this.projectRootPath, filename);
+        return path.join(this.projectRootPath, filename).replace(DBL_SLASHES, "/");


Please don't use string replace to normalize paths. Use path.posix.join instead

replace is faster than split + path.posix.join, and with leading slash

Ok, It appears that path.join would not correct path separators.

vladimir-kotikov · 2017-07-25T10:54:15Z

-                // Not in a react-native project
-                return false;
-            });
+    private getFromExpConfig<T>(key: string): Q.Promise<T> {


I'm curious why is it really required to use generics? Semantically I'd rather return any and then cast result to expected type

vladimir-kotikov · 2017-07-25T11:02:11Z

+    private detectEntry(): Q.Promise<string> {
+        this.lazilyInitialize();
+        return Q.all([
+            this.fs.exists(this.pathToFileInWorkspace(DEFAULT_EXPONENT_INDEX)),


Instead of checking some predefined "magic" locations you might want to look at "main" entry in package.json. I ddn't find any documentation but from my experience it always points to the main entry point. Also note the case when for example "main" points to non-existent file - in that case you'll need to check for ios and adroid variants of that file, i.e.

index.js (doesn't exist) -> index.ios.js / index.android.js

It's just a bit refactoring for legacy code. And we discussed about that.

vladimir-kotikov · 2017-07-25T11:14:24Z

-                const expJsonPath = this.pathToFileInWorkspace(EXP_JSON);
-                if (!this.fileSystem.existsSync(expJsonPath) || exponentJson.overwriteExpJson) {
+    private patchAppJson(isExpo: boolean = true): Q.Promise<void> {
+        return this.readAppJson()


I can't find .catch for this. What happens if neither app.json nor exp.json exists

It's incorrect app, and we'll crash, i think it's ok

I can see at least one code path where the exception here will not be caught by caller code which likely would lead to extension crash - this is a bad UX and must be avoided if possible.

Have you tested this scenario?

itoys added 2 commits July 18, 2017 11:35

Code refactoring for better Expo support

8e7a8fa

Merge branch 'master' of https://github.com/Microsoft/vscode-react-na…

73711eb

…tive into expo-support-upgrade

msftclas added the cla-already-signed label Jul 18, 2017

Code refactoring for better Expo support

4f7b30b

itoys force-pushed the expo-support-upgrade branch from 4c58811 to 4f7b30b Compare July 19, 2017 12:06

vladimir-kotikov suggested changes Jul 25, 2017

View reviewed changes

itoys added 2 commits July 25, 2017 15:50

Fix issue after review

4cabcbe

Fix issue after review

87f1c60

vladimir-kotikov approved these changes Jul 25, 2017

View reviewed changes

Merge branch 'master' into expo-support-upgrade

0cc3b61

vladimir-kotikov merged commit 94cd514 into microsoft:master Jul 26, 2017

Conversation

itoys commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Jul 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itoys Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

itoys commented Jul 18, 2017 •

edited

Loading

itoys Jul 25, 2017 •

edited

Loading